Skip to content

Comments

Adds Linear, Miro, and Webflow web providers#2284

Merged
kevinchalet merged 9 commits intoopeniddict:devfrom
jerriep:add-linear-web-provider
Mar 28, 2025
Merged

Adds Linear, Miro, and Webflow web providers#2284
kevinchalet merged 9 commits intoopeniddict:devfrom
jerriep:add-linear-web-provider

Conversation

@jerriep
Copy link
Contributor

@jerriep jerriep commented Mar 26, 2025

This adds the web providers for Linear, Miro, and Webflow.

Linear

image

Miro

image

Webflow

image

@jerriep jerriep marked this pull request as ready for review March 26, 2025 08:27
@jerriep jerriep requested a review from kevinchalet March 26, 2025 08:28
Copy link
Member

@kevinchalet kevinchalet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks for your PR, @jerriep! ❤️

// Miro uses a JSON payload that expects the "accessToken", "clientId", and "clientSecret" properties
else if (context.Registration.ProviderType is ProviderTypes.Miro)
{
context.Request["accessToken"] = context.Token;
Copy link
Member

@kevinchalet kevinchalet Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're going to send the access token in the Authorization header, there's no real point mapping it here. And if you don't nullify context.Request.Token here, you can reuse the same branch as the Zendesk provider in the AttachBearerAccessToken handler 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Miro requires both the accessToken property in the JSON payload and the Bearer token. If I send just the Bearer token without the accessToken property, I get an error:

image

Likewise, if I omit the Bearer token and send just the accessToken property, it fails as well:

image

I need to supply both to get a successful response:

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, I thought that branch was for the Linear provider... that uses a single access_token parameter 🤣

else if (context.Registration.ProviderType is ProviderTypes.Miro)
{
context.Request["accessToken"] = context.Token;
context.Request["clientId"] = context.Request.ClientId;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I don't see these parameters mentioned in https://developers.linear.app/docs/oauth/authentication#id-6.-revoke-an-access-token. Are we sure their non-standard revocation endpoint supports client authentication?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, Linear is an interesting one. Before I updated the parameters according to their docs, I tested the revocation and it worked:

image

So, in the initial PR, I did not make the code changes according to their docs. But, it is clearly risky doing this as it is not documented to work this way, and they can easily change it in the future so it does not work correctly anymore.

I have now updated the code to conform to their docs:

image

// In the case of Miro, we renamed the Token to accessToken, so we cannot use the Token property.
else if (context.Registration.ProviderType is ProviderTypes.Miro)
{
if (context.Request.TryGetParameter("accessToken", out var parameter) && parameter.GetRawValue() is string accessToken)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll be able to remove that branch so it's not really important, but FYI, using GetRawValue() is discouraged as there's no absolute guarantee a parameter will be backed by a CLR string value: here, we use context.Request["accessToken"] = context.Token earlier, so it may indeed be backed by a string, but a custom event handler registered by the user could change that to say, a JsonValue representing a string:

context.Request["accessToken"] = JsonValue.Create(context.Token);

It's better to always use the conversion operators so that the OpenIddictParameter structure gives you the correct target type independently of its backing CLR type:

var token = (string?) context.Request["access_token"];
if (!string.IsNullOrEmpty(token))
{
    request.Headers.Authorization = new AuthenticationHeaderValue(Schemes.Bearer, token);
}

... or if you prefer the minimally-indented version... 😄

else if (context.Registration.ProviderType is ProviderTypes.Miro &&
    (string?) context.Request["access_token"] is { Length: > 0 } token)
{
    request.Headers.Authorization = new AuthenticationHeaderValue(Schemes.Bearer, token);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since both the accessToken property in the JSON payload, as well as the Bearer token are required, I fixed this code as per your suggestion.

@kevinchalet
Copy link
Member

Note: if we can merge this PR this week, I'll make sure it's included in the 6.2.0 release 😃

@jerriep
Copy link
Contributor Author

jerriep commented Mar 27, 2025

Hey @kevinchalet, I believe I addressed all the points you raised. I also picked up that the Miro and Webflow nameidentifier claim was not resolving correctly, so I fixed that as well. I did a final test of each provider doing the authorization and revocation and everything works as expected.

Please review again

Comment on lines 129 to 130
context.Request.ClientId = null;
context.Request.ClientSecret = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works just fine, but there's actually a cleaner way to solve this problem: letting OpenIddict know that the revocation endpoint doesn't support client authentication so that it doesn't even attach the credentials to the request.

For that, you'll just need to add <RevocationEndpointAuthMethod Value="none" /> under the provider configuration (if you get a XML schema warning telling you the none value isn't allowed, you can rebase your branch, I tweaked the schema yesterday to allow all supported values).

<Configuration AuthorizationEndpoint="https://linear.app/oauth/authorize"
               TokenEndpoint="https://api.linear.app/oauth/token"
               RevocationEndpoint="https://api.linear.app/oauth/revoke"
               UserInfoEndpoint="https://api.linear.app/graphql">
  <RevocationEndpointAuthMethod Value="none" />
</Configuration>

If you do that, you should be able to merge that else if branch with the previous one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the <RevocationEndpointAuthMethod Value="none" /> and merged the logic branch with the previous one. That caused the client_secret not to be sent, but it still sent the client_id.

image

So, I decided to keep the Linear logic in its own branch and clear the client_id by explicitly setting ClientId to null.

image

@kevinchalet
Copy link
Member

Please review again

Looks great. Just a minor improvement remaining and it should be good to merge 😃

jerriep added 8 commits March 28, 2025 06:38
- Adds additional Miro claims for team and organization
- Resolves Miro and Webflow nameidentifier claim correctly
- Sorts Linear UserFields values alphabetically in XML file
- Fixes incorrect use of OpenIddictParameter.GetRawValue()
- Sends documented parameters to Linear token revocation endpoint
@jerriep jerriep force-pushed the add-linear-web-provider branch from 4c5b4af to 50f538c Compare March 27, 2025 23:56
@jerriep
Copy link
Contributor Author

jerriep commented Mar 28, 2025

I did another final run through the authorization and revocation flow for all three providers and everything still works. This PR should be good to go.

@kevinchalet kevinchalet merged commit 8ecee02 into openiddict:dev Mar 28, 2025
6 checks passed
@kevinchalet
Copy link
Member

Merged, thanks for your contribution! ❤️

(I'll backport your PR to 6.2.0 and release it later today)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants